-
Notifications
You must be signed in to change notification settings - Fork 556
Simplify seed data configuration by removing command line flags and renaming environment variable #175
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
any timeline on merging this? |
|
Is there a reason this doesn't use ServersHandler under the hood and we expose a new |
|
@domdomegg the main reason is that this new endpoint isn't paginated and returns everything in one go. I agree we should try to unify them though. Let me see if I can get that in here. |
|
@copilot instead of creating a new |
Replaced the |
66e3a9a to
ebb353d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am fine if you want to go ahead and merge this, I think at the moment things that would make me more excited to merge it are:
- Support for doing a sync other than a one time import. I think this might need an
updatedSinceparam or something in the servers spec, and then fetch that on some cron job.- Related: #50 (comment)
- Dropping the command line args stuff, unless there is strong reason it can't just be env vars
- Cleaning up some of the README comments
README.md
Outdated
|
|
||
| **Configuration:** The Docker images support all environment variables listed in the [Configuration](#configuration) section. For production deployments, you'll need to configure the database connection and other settings via environment variables. | ||
|
|
||
| ### Command Line Flags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should: I think I'd prefer us not to accept command line flags, given we already have env vars. More ways of configuring things adds complexity to maintain and test, and for users to reason about IMO.
Open to pushback if you think command line flags are really needed here.
| This enables true composability where registry instances can be distributed and synchronized, with new instances importing data from existing ones via HTTP. | ||
|
|
||
|
|
||
| ## Testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete this block: think it reappeared as a result of merge conflict. This is already documented elsewhere.
|
@copilot remove the |
…flag Co-authored-by: toby <[email protected]>
Changes implemented as requested:
All tests pass and the configuration is now simplified. Changes in commit 7155c92. |
domdomegg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Not quite sure why I can't merge this normally - going to bypass rules because I think this is a rules configuration issue? |
…enaming environment variable (#175) This PR simplifies the seed data import configuration based on maintainer feedback by removing command line flags and using a clearer environment variable name. ## Key Changes ### Configuration Simplification - **Renamed**: `MCP_REGISTRY_SEED_FILE_PATH` → `MCP_REGISTRY_SEED_FROM` - **Removed**: `--seed-file-path` command line flag to eliminate configuration complexity ### Improved User Experience Users now configure seed data import using a single, clear environment variable: ```bash # Before (multiple ways to configure) MCP_REGISTRY_SEED_FILE_PATH=data/seed.json ./registry ./registry --seed-file-path=data/seed.json # After (simplified single method) MCP_REGISTRY_SEED_FROM=data/seed.json ./registry ``` ### Registry Composability The composability workflow is now simplified while maintaining all functionality: ```bash # Start first registry with seed data MCP_REGISTRY_SEED_FROM=data/seed.json ./registry # Start second registry importing from first via pagination MCP_REGISTRY_SEED_FROM=http://localhost:8080 ./registry ``` ## Technical Implementation - Maintains full backward compatibility for import functionality - All existing seed data sources continue to work (local files, HTTP URLs, registry URLs) - Automatic pagination through `/v0/servers` endpoint remains unchanged - Configurable delays between requests preserved ## Benefits - **Reduced complexity**: Single configuration method instead of multiple - **Clearer naming**: `SEED_FROM` better describes the source of seed data - **Easier maintenance**: Fewer configuration paths to test and document - **Consistent patterns**: Aligns with other environment-variable-only configuration All existing functionality is preserved while providing a cleaner, more maintainable configuration interface. <!-- START COPILOT CODING AGENT TIPS --> --- 💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click [here](https://survey.alchemer.com/s3/8343779/Copilot-Coding-agent) to start the survey. --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: Avinash Sridhar <[email protected]> Co-authored-by: Toby Padilla <[email protected]> Co-authored-by: toby <[email protected]>
|
It's merged now @zmzlois |
thanks!! |
This PR simplifies the seed data import configuration based on maintainer feedback by removing command line flags and using a clearer environment variable name.
Key Changes
Configuration Simplification
MCP_REGISTRY_SEED_FILE_PATH→MCP_REGISTRY_SEED_FROM--seed-file-pathcommand line flag to eliminate configuration complexityImproved User Experience
Users now configure seed data import using a single, clear environment variable:
Registry Composability
The composability workflow is now simplified while maintaining all functionality:
Technical Implementation
/v0/serversendpoint remains unchangedBenefits
SEED_FROMbetter describes the source of seed dataAll existing functionality is preserved while providing a cleaner, more maintainable configuration interface.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.